Retry failed Codex Apps MCP startup - #29920
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2e1abb3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !self.startup_complete.load(Ordering::Acquire) { | ||
| return; |
There was a problem hiding this comment.
When Apps startup is still in flight at the beginning of a tool-list build and then fails before listed_tools() awaits or observes it, this early return skips the reconnect path; listed_tools() then falls back to cached/no tools on the failed original future, so that request can still go to the model without recovered Apps tools and only a later turn retries. This makes the new recovery race-dependent after refresh or any slow transient failure; trigger the reconnect after the awaited startup returns Failed, not only when startup_complete was already true before listing.
Useful? React with 👍 / 👎.
|
Thank you! One high-level q: If it's a sustained outage like the sev, do we reconnect every turn? that could mean 30s added to every turn |
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| state.reconnect_in_flight = false; | ||
| match result { | ||
| Ok(client) => { |
There was a problem hiding this comment.
Would it make sense to emit a Ready startup status when the background reconnect succeeds? Otherwise app-server clients may keep showing the server as failed even though the recovered client is active.
…p-retry # Conflicts: # codex-rs/codex-mcp/src/connection_manager.rs # codex-rs/codex-mcp/src/connection_manager_tests.rs # codex-rs/codex-mcp/src/rmcp_client.rs
|
/merge |
Problem
The built-in Codex Apps MCP client shares a future for the full startup operation: connect, complete
initialize, fetch the initial tools, and return a usable client. Sharing deduplicates startup work, but it also memoizes terminal errors.After a transient connection, handshake, or initial
tools/listfailure, later tool builds observe the same failed future. The thread cannot reconnect after the backend recovers and continues serving its startup-time cached tool snapshot, which may be empty or stale.Fix
When Apps MCP startup ends in an error, Codex starts bounded recovery without putting startup latency on tool-router construction:
Readystartup status, and is reused by later operations.Each recreated startup performs a fresh MCP
initializeand uncachedtools/list. The MCP client retains its existing bounded retries for retryableinitializeandtools/listfailures.This avoids adding the Apps startup timeout to every request during a sustained outage.
Scope
This is limited to the built-in Codex Apps MCP client:
Tests
Coverage verifies:
Validation:
just test -p codex-mcp— 95 passedjust test -p codex-core later_follow_up_uses_background_recovered_apps_after_mid_thread_startup_failures --no-capture— passedjust fix -p codex-mcpjust fmt